[MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster #2254

maobaolong · 2024-11-18T08:43:23Z

What changes were proposed in this pull request?

put sparkConf as extra properties while client request accessCluster

Why are the changes needed?

Coordinator can let the spark application access rss cluster or not by some customized access checker leverage some of the spark configs.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

No need

github-actions · 2024-11-18T09:10:56Z

Test Results

2 956 files +16 2 956 suites +16 6h 27m 34s ⏱️ + 17m 2s
1 092 tests ± 0 1 090 ✅ + 2 2 💤 ±0 0 ❌ - 1
13 685 runs +22 13 655 ✅ +24 30 💤 ±0 0 ❌ - 1

Results for commit 8d3f2ac. ± Comparison against base commit 54611f3.

♻️ This comment has been updated with latest results.

zuston

LGTM. Although putting all confs into the general properites looks a little bit strange

jerqi · 2024-11-19T03:02:41Z

client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

@@ -123,6 +123,7 @@ private boolean tryAccessCluster() {
    Map<String, String> extraProperties = Maps.newHashMap();
    extraProperties.put(
        ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    extraProperties.putAll(RssSparkConfig.sparkConfToMap(sparkConf));


We shouldn't put Spark configurations to coordinator. Coordinator isn't related to compute framework. The spark configurations is coupled with Spark framework. You can have an extractor instead of passing them directly.

This PR is related to #2255 , I want to determine whether to ban an app by a specific ban id which extract from one of the spark client config, but I don't want client config it concretely.

I'm not sure whether it is good design to have a ban id list. It seems that it isn't common enough.

It's not common if bannedIdProvider contains spark.

maobaolong · 2024-11-20T15:20:55Z

@jerqi Thanks for your suggestion, removed the spark. prefix to be more generic by new commit

jerqi · 2024-11-22T10:27:50Z

client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)


This is too tricky....

Em... Thanks for your review! I'm sorry for this.

I would greatly appreciate it if you could give me some advice.

You can implement a white list or configuration filter. The white list can be configured in the configuration. The configuration filter could have length configuration filter. The length could be configured in the configuration.

Do you think use black list instead of white list can be possible?

Ok for me, too. black list would better to be replaced by excludeList.

maobaolong · 2024-11-25T06:36:40Z

@jerqi Exclude config added, PTAL

jerqi · 2024-11-25T07:12:31Z

client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

+
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);


The default is null? Will it cause an error?

Thanks for your tip, I set a default value.

jerqi · 2024-11-25T10:39:06Z

common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java

@@ -307,6 +307,6 @@ public class RssClientConf {
      ConfigOptions.key("rss.client.reportExcludeProperties")
          .stringType()
          .asList()
-          .noDefaultValue()
+          .defaultValues("hdfs.inputPaths")


Em... this is a weird value.

codecov-commenter · 2024-11-25T11:31:16Z

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 52.36%. Comparing base (477bc30) to head (9a3b25d).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
.../java/org/apache/spark/shuffle/RssSparkConfig.java	20.00%	4 Missing ⚠️
...uniffle/shuffle/manager/RssShuffleManagerBase.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2254      +/-   ##
============================================
- Coverage     52.65%   52.36%   -0.29%     
+ Complexity     3376     2969     -407     
============================================
  Files           514      466      -48     
  Lines         27565    22326    -5239     
  Branches       2582     2044     -538     
============================================
- Hits          14513    11691    -2822     
+ Misses        12092     9890    -2202     
+ Partials        960      745     -215

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

jerqi · 2024-11-25T11:36:27Z

client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    rssConf.getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)


Should you remove <100?

jerqi · 2024-11-25T11:36:28Z

client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
+    // Put all spark conf into extra properties, except which length is longer than 100


Should you remove the comment?

Put sparkConf as extra properties while client request accessCluster

827ee04

maobaolong force-pushed the putSparkConfWhileAccessCluster branch from e7e5f4f to 827ee04 Compare November 18, 2024 08:44

zuston previously approved these changes Nov 19, 2024

View reviewed changes

jerqi reviewed Nov 19, 2024

View reviewed changes

Address suggestion, remove the spark prefix to be common

f538b56

maobaolong dismissed zuston’s stale review via f538b56 November 20, 2024 15:16

maobaolong requested a review from jerqi November 20, 2024 15:20

jerqi reviewed Nov 22, 2024

View reviewed changes

jerqi reviewed Nov 25, 2024

View reviewed changes

Add a exclude properties config

9b01609

maobaolong force-pushed the putSparkConfWhileAccessCluster branch from 746adcf to 9b01609 Compare November 25, 2024 08:24

Set default value

1c93abf

jerqi reviewed Nov 25, 2024

View reviewed changes

Remove weired config value to keep empty list by default

9a3b25d

jerqi reviewed Nov 25, 2024

View reviewed changes

Remove larger than 100 filter

8d3f2ac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster #2254

[MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster #2254

maobaolong commented Nov 18, 2024

github-actions bot commented Nov 18, 2024 •

edited

Loading

zuston left a comment

jerqi Nov 19, 2024

maobaolong Nov 19, 2024

jerqi Nov 19, 2024

jerqi Nov 19, 2024

maobaolong commented Nov 20, 2024

jerqi Nov 22, 2024

maobaolong Nov 22, 2024

jerqi Nov 25, 2024

maobaolong Nov 25, 2024

jerqi Nov 25, 2024 •

edited

Loading

maobaolong Nov 25, 2024

maobaolong commented Nov 25, 2024

jerqi Nov 25, 2024

maobaolong Nov 25, 2024

jerqi Nov 25, 2024

codecov-commenter commented Nov 25, 2024

jerqi Nov 25, 2024

maobaolong Nov 25, 2024

jerqi Nov 25, 2024

maobaolong Nov 25, 2024

[MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster #2254

Are you sure you want to change the base?

[MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster #2254

Conversation

maobaolong commented Nov 18, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

github-actions bot commented Nov 18, 2024 • edited Loading

Test Results

zuston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 25, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2024 •

edited

Loading

jerqi Nov 25, 2024 •

edited

Loading